Skip to content

refactor: [M3-9647] - Reduce api requests made for every keystroke in Volume attach drawer #12052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

harsh-akamai
Copy link
Contributor

@harsh-akamai harsh-akamai commented Apr 17, 2025

Description πŸ“

CM fires an API request for every keystroke in Volume attach drawer.

Changes πŸ”„

  • Refactored the query to only run in the initial render

Target release date πŸ—“οΈ

N/A

Preview πŸ“·

Before After
Screen.Recording.2025-04-17.at.5.02.43.PM.mov
Screen.Recording.2025-04-17.at.5.07.32.PM.mov

How to test πŸ§ͺ

Reproduction steps

  • Navigate to "Add Volume" drawer under Linode Storage
  • Switch to "Attach" mode
  • Type anything on the Volume Select and see a /volumes request fired on each key store

Verification steps

  • Check that the /volumes request is only fired on the initial render of the drawer
Author Checklists

As an Author, to speed up the review process, I considered πŸ€”

πŸ‘€ Doing a self review
❔ Our contribution guidelines
🀏 Splitting feature into small PRs
βž• Adding a changeset
πŸ§ͺ Providing/improving test coverage
πŸ” Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
πŸ‘£ Providing comprehensive reproduction steps
πŸ“‘ Providing or updating our documentation
πŸ•› Scheduling a pair reviewing session
πŸ“± Providing mobile support
β™Ώ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed βœ…

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@harsh-akamai harsh-akamai self-assigned this Apr 17, 2025
const { data: volume } = useVolumeQuery(
values.volume_id,
values.volume_id !== -1
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabled the useVolumeQuery only when Volume_id is set and not -1(default value)

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

πŸ”Ί 2 failing tests on test run #2 β†—οΈŽ

❌ Failingβœ… Passingβ†ͺ️ SkippedπŸ• Duration
2 Failing538 Passing4 Skipped128m 31s

Details

Failing Tests
SpecTest
❌smoke-community-stackscripts.spec.tsCloud Manager Cypress Testsβ†’Community Stackscripts integration tests Β» Community Stackscripts integration tests
❌bucket-details-multicluster.spec.tsCloud Manager Cypress Testsβ†’Object Storage Multicluster Bucket Details Tabsβ†’Properties tab without required capabilities Β» Object Storage Multicluster Bucket Details Tabsβ†’Properties tab without required capabilities

Troubleshooting

Use this command to re-run the failing tests:

pnpm cy:run -s "cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts,cypress/e2e/core/objectStorageMulticluster/bucket-details-multicluster.spec.ts"

@harsh-akamai harsh-akamai marked this pull request as ready for review April 17, 2025 12:55
@harsh-akamai harsh-akamai requested a review from a team as a code owner April 17, 2025 12:55
@harsh-akamai harsh-akamai requested review from dwiley-akamai and hasyed-akamai and removed request for a team April 17, 2025 12:55
Comment on lines -20 to -27
const searchFilter = inputValue
? {
'+or': [
{ label: { '+contains': inputValue } },
{ tags: { '+contains': inputValue } },
],
}
: {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the API searching functionality makes it so that if the user searches for a result that isn't on the first page of paginated results, the result does not show.

Screen.Recording.2025-04-17.at.10.15.43.AM.mov

We need to decide if we want to keep the API pagination/filtering or go full client side search/filtering. My vote would be to keep this component using API pagination/filtering.

Even better, we could use @linode/search to provide the user even more filtering functionality as an easter egg πŸ₯š

Screenshot 2025-04-17 at 10 24 27β€―AM

The implementation would look something like

 const [inputValue, setInputValue] = React.useState<string>('');

  const debouncedSearchValue = useDebouncedValue(inputValue);

  const { filter: searchFilter } = getAPIFilterFromQuery(debouncedSearchValue, {
    searchableFieldsWithoutOperator: ['label', 'tags'],
  });

  const { data, fetchNextPage, hasNextPage, isLoading } =
    useInfiniteVolumesQuery({
      ...searchFilter,
      ...(region ? { region } : {}),
      '+order': 'asc',
      // linode_id: null,  <- if the API let us, we would do this
      '+order_by': 'label',
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if I'm using API side filtering instead of client side filtering, the searched/user-typed option is not shown if it is not present on the first page πŸ€”

Screen.Recording.2025-04-22.at.11.16.02.PM.mov

I'm not able to think about an approach to solve this other than fetching all pages data at once(which obviously isn't very efficient)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can get things working better by fine turning when we call setInputValue("")?

Code
import { useInfiniteVolumesQuery } from '@linode/queries';
import { getAPIFilterFromQuery } from '@linode/search';
import { Autocomplete } from '@linode/ui';
import { useDebouncedValue } from '@linode/utilities';
import * as React from 'react';

interface Props {
  disabled?: boolean;
  error?: string;
  name: string;
  onBlur: (e: any) => void;
  onChange: (volumeId: null | number) => void;
  region?: string;
  value: number;
}

export const VolumeSelect = (props: Props) => {
  const { disabled, error, name, onBlur, onChange, region, value } = props;

  const [inputValue, setInputValue] = React.useState<string>('');

  const debouncedSearchValue = useDebouncedValue(inputValue);

  const { filter: searchFilter } = getAPIFilterFromQuery(debouncedSearchValue, {
    searchableFieldsWithoutOperator: ['label', 'tags'],
  });

  const { data, fetchNextPage, hasNextPage, isLoading } =
    useInfiniteVolumesQuery({
      ...searchFilter,
      ...(region ? { region } : {}),
      '+order': 'asc',
      // linode_id: null,  <- if the API let us, we would do this
      '+order_by': 'label',
    });

  const options = data?.pages
    .flatMap((page) => page.data)
    .map(({ id, label }) => ({ id, label }));

  const selectedVolume = options?.find((option) => option.id === value) ?? null;

  return (
    <Autocomplete
      disabled={disabled}
      errorText={error}
      helperText={
        region && "Only volumes in this Linode's region are attachable."
      }
      id={name}
      inputValue={selectedVolume ? selectedVolume.label : inputValue}
      isOptionEqualToValue={(option) => option.id === selectedVolume?.id}
      label="Volume"
      ListboxProps={{
        onScroll: (event: React.SyntheticEvent) => {
          const listboxNode = event.currentTarget;
          if (
            listboxNode.scrollTop + listboxNode.clientHeight >=
              listboxNode.scrollHeight &&
            hasNextPage
          ) {
            fetchNextPage();
          }
        },
      }}
      loading={isLoading}
      onBlur={onBlur}
      onChange={(event, value) => {
        onChange(value?.id ?? null);
      }}
      onInputChange={(event, value, reason) => {
        if (reason === 'input') {
          setInputValue(value);
        }
        if (reason === 'clear' || reason === 'blur') {
          setInputValue('');
        }
      }}
      options={options ?? []}
      placeholder="Select a Volume"
      value={selectedVolume}
    />
  );
};

If that doesn't work, maybe we can find a way to make our API filter include the currently selected volume even when the inputValue is ""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also in favor of retaining API pagination/filtering in this component

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that code I provided has too many edge cases / issues, I have another idea πŸ’‘

@@ -47,6 +32,15 @@ export const VolumeSelect = (props: Props) => {

return (
<Autocomplete
disabled={disabled}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we retain API filtering, we should add this prop

Suggested change
disabled={disabled}
filterOptions={(options, state) => options

to the Autocomplete to ensure it doesn't do any client side filtering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants